Skip to content

[16.0][IMP]fastapi: expose FastAPI docs only if field is set#537

Merged
OCA-git-bot merged 1 commit into
OCA:16.0from
PyTech-SRL:16.0-hide-api-docs
Jan 5, 2026
Merged

[16.0][IMP]fastapi: expose FastAPI docs only if field is set#537
OCA-git-bot merged 1 commit into
OCA:16.0from
PyTech-SRL:16.0-hide-api-docs

Conversation

@PicchiSeba

@PicchiSeba PicchiSeba commented Jun 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

If we go into production right now, the FastAPI documentation endpoints (Swagger, Redoc, OpenAPI) are reachable by anyone.

There might be cases where this feature is not required or even prohibited.

With this PR we aim to hide these interfaces with a System Parameter

@OCA-git-bot

OCA-git-bot commented Jun 11, 2025

Copy link
Copy Markdown
Contributor

Hi @lmignon,
some modules you are maintaining are being modified, check this out!

lmignon
lmignon requested changes Jun 11, 2025
View reviewed changes

@lmignon lmignon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer a configuration parameter on the endpoint in place of a global config parameter.

@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch 2 times, most recently from dcc5ceb to 881bfdc Compare June 11, 2025 12:01
@PicchiSeba

PicchiSeba commented Jun 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

While testing it I found a problem, not necessarily related to this implementation.

If I enable the endpoint with expose_fastapi_docs set to True the docs are reachable as expected.
Then if I change expose_fastapi_docs to `False the docs are still reachable.

I found that the _prepare_fastapi_app_params method is never triggered after synching the endpoint. Perhaps it gets computed only before mounting the FastAPI app?

@PicchiSeba PicchiSeba changed the title [IMP]fastapi: expose FastAPI docs only if system parameter is set [IMP]fastapi: expose FastAPI docs only if field is set Jun 11, 2025
@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch from 881bfdc to de344b4 Compare June 11, 2025 12:14
@lmignon

lmignon commented Jun 11, 2025

Copy link
Copy Markdown
Contributor

While testing it I found a problem, not necessarily related to this implementation.

If I enable the endpoint with expose_fastapi_docs set to True the docs are reachable as expected. Then if I change expose_fastapi_docs to `False the docs are still reachable.

I found that the _prepare_fastapi_app_params method is never triggered after synching the endpoint. Perhaps it gets computed only before mounting the FastAPI app?

It's because you must add the new field into the list returned by the method _fastapi_app_fields. This will instruct the addon that the app must be rebuild when you new field is modified.

@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch from de344b4 to 649b406 Compare June 12, 2025 07:19
@PicchiSeba PicchiSeba changed the title [IMP]fastapi: expose FastAPI docs only if field is set [16.0][IMP]fastapi: expose FastAPI docs only if field is set Jun 13, 2025
SirPyTech
SirPyTech suggested changes Jun 13, 2025
View reviewed changes

@SirPyTech SirPyTech left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!
Good work 👏
I just have a few suggestions, mainly about documentation of the code, please have a look at the comments below.

Comment thread fastapi/models/fastapi_endpoint.py Outdated
Comment thread fastapi/tests/test_fastapi.py Outdated
Comment thread fastapi/tests/test_fastapi.py
@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch 2 times, most recently from 85bda5a to 8ca9d00 Compare June 13, 2025 10:02
SirPyTech
SirPyTech approved these changes Jun 13, 2025
View reviewed changes
Comment thread fastapi/tests/test_fastapi.py
Comment thread fastapi/models/fastapi_endpoint.py Outdated
Comment thread fastapi/tests/test_fastapi.py Outdated
lmignon
lmignon approved these changes Jun 13, 2025
View reviewed changes

@lmignon lmignon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PicchiSeba Thank you once again. LGTM

@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch from 8ca9d00 to 95c8ca1 Compare June 13, 2025 15:49
@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch from 95c8ca1 to 4eea801 Compare June 13, 2025 15:50
HekkiMelody
HekkiMelody approved these changes Jun 16, 2025
View reviewed changes

@HekkiMelody HekkiMelody left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review, LGTM

@OCA-git-bot

OCA-git-bot commented Jun 16, 2025

Copy link
Copy Markdown
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@sebastienbeau sebastienbeau added this to the 16.0 milestone Sep 1, 2025
@github-actions

github-actions Bot commented Jan 4, 2026

Copy link
Copy Markdown

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions Bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jan 4, 2026
@lmignon

lmignon commented Jan 5, 2026

Copy link
Copy Markdown
Contributor

/ocabot merge major

@OCA-git-bot

OCA-git-bot commented Jan 5, 2026

Copy link
Copy Markdown
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-537-by-lmignon-bump-major, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jan 5, 2026
@OCA-git-bot
Merge PR #537 into 16.0
e634810 
Signed-off-by lmignon
@OCA-git-bot

OCA-git-bot commented Jan 5, 2026

Copy link
Copy Markdown
Contributor

It looks like something changed on 16.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 16.0-ocabot-merge-pr-537-by-lmignon-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e0e4fb8 into OCA:16.0 Jan 5, 2026
6 of 7 checks passed
@OCA-git-bot

OCA-git-bot commented Jan 5, 2026

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 03121f3. Thanks a lot for contributing to OCA. ❤️

@HekkiMelody HekkiMelody deleted the 16.0-hide-api-docs branch January 5, 2026 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lmignon lmignon lmignon approved these changes
+2 more reviewers
@HekkiMelody HekkiMelody HekkiMelody approved these changes
@SirPyTech SirPyTech SirPyTech approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved merged 🎉 ready to merge stale PR/Issue without recent activity, it'll be soon closed automatically.

Projects

None yet

Milestone

16.0

Development

Successfully merging this pull request may close these issues.

6 participants

@PicchiSeba @OCA-git-bot @lmignon @HekkiMelody @SirPyTech @sebastienbeau